Add TO Go cdns/health and cdns/{name}/health by rob05c · Pull Request #2305 · apache/trafficcontrol

rob05c · 2018-05-19T23:45:05Z

Rewrites /cdns/health and /cdns/{{name}}/health to Go. Closes #3778 and closes #3779 .

asfgit · 2018-05-19T23:53:46Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/incubator-trafficcontrol-PR/1623/
Test PASSed.

asfgit · 2018-05-20T00:43:37Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/incubator-trafficcontrol-PR/1624/
Test PASSed.

asfgit · 2018-05-22T15:57:32Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/incubator-trafficcontrol-PR/1646/
Test PASSed.

asfgit · 2018-06-05T15:25:25Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/incubator-trafficcontrol-PR/1761/
Test PASSed.

asfgit · 2018-06-23T16:24:31Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/1896/
Test PASSed.

asfgit · 2018-06-23T17:24:02Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/1897/
Test PASSed.

asfgit · 2018-06-23T20:10:00Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/1900/
Test PASSed.

asfgit · 2018-06-25T17:25:31Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/1907/
Test PASSed.

dneuman64 · 2018-06-25T22:09:12Z

Do we think this should be a config? I have seen several times now where the type "RASCAL" was hard-coded and we had to change our TM type. IMO you should be able to make the type whatever you want and it should work.

I think it should definitely be a config, once we've moved everything to Go. But right now, it's still hard-coded all over Perl.

I don't think we should make a config yet, which would make people think they could change it, when it'd still break to be anything but "RASCAL".

But definitely +100 when it's all Go, and we can set one variable. I've wanted to change our db "RASCAL" type to something like "monitor" for ages.

dneuman64 · 2018-06-25T22:15:36Z

I would expect a heathtest.go to be a part of this PR is there a reason it is not included?

rob05c · 2018-06-26T20:03:55Z

Not sure what you mean. What would be in healthtest.go?

asfgit · 2018-07-05T19:56:53Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/1998/
Test PASSed.

asfgit · 2018-07-09T17:09:46Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/2039/
Test PASSed.

asfgit · 2018-07-19T02:41:28Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/2114/
Test FAILed.

asf-ci · 2019-09-16T14:49:58Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/4256/
Test PASSed.

mhoppa

One minor comment - also should we be adding tests?

mhoppa · 2019-09-16T14:59:49Z

Given this looks like duplicate logic from https://github.com/apache/trafficcontrol/blob/master/traffic_ops/traffic_ops_golang/cdn/capacity.go as well as https://github.com/apache/trafficcontrol/blob/master/traffic_ops/traffic_ops_golang/cachesstats/cachesstats.go#L202 should you go ahead and replace the other functions with this utility function?

Yes, they were copy-pasted, because the PRs were open at the same time, with the intention of combining them after one was merged to master. Good catch. I'll do that now.

Done. Also with GetCRConfig.

rob05c · 2019-09-16T15:30:52Z

also should we be adding tests?

This endpoint calls out to Traffic Monitor. The "API Test" framework doesn't currently support Monitors. It's also not really possible to unit test; we could mock everything, but it wouldn't be testing much of anything at that point (which is why we've been doing "API Tests" for TO).

We should definitely test this (and other Monitor endpoints) when the API Tests support it. But it'll be a nontrivial amount of work to add that support.

I just made #3924 - we had one for Riak, but not Monitors.

mitchell852 · 2019-09-18T18:53:51Z

@rob05c and @mhoppa - i believe this PR is going to have the same problem that was addressed in #3927 , right?

mhoppa · 2019-09-19T17:49:13Z

Yes it will have the same problem

there is a bug

mitchell852

@rob05c - i think this PR needs the changes that were made in #3927

rob05c · 2019-09-27T19:35:43Z

I'd like to look into #3927 first. Not sure when I'll have time.

It shouldn't be necessary, HTTP/2 is backwards-compatible by design. If a client tries to use H2, and a server doesn't support it, it automatically falls back to H1. If it doesn't, the Server is violating HTTP/1.1. So, it shouldn't ever be necessary to forcibly disable it. I'd like to at least understand why it's necessary.

asf-ci · 2019-12-11T16:52:44Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/4908/

rob05c · 2019-12-11T17:10:32Z

Ok, I investigated, determined Go 1.13 http.Transport fundamentally doesn't support H2 Proxies. I added the code from #3927 and a comment to that effect. See golang/go#26479 "We only support http1 proxies currently."

Rebased; manually tested again, still works as expected, still matches Perl.

asf-ci · 2019-12-11T21:28:34Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/4910/

asf-ci · 2019-12-11T21:57:06Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/4913/

mhoppa

code looks good and tests locally good

mitchell852

appears to work as the Perl did.

rob05c added new feature A new feature, capability or behavior Traffic Ops API (golang) labels May 19, 2018

rob05c mentioned this pull request May 19, 2018

Rewrite Traffic Ops CRConfig Table Endpoints in Go #2232

Closed

rob05c force-pushed the to-go-cdnhealth branch from b3642cd to 3dfe47a Compare May 20, 2018 00:38

rob05c force-pushed the to-go-cdnhealth branch from 3dfe47a to 33a135f Compare May 22, 2018 15:00

rob05c force-pushed the to-go-cdnhealth branch from 33a135f to a650da2 Compare June 5, 2018 14:49

rob05c force-pushed the to-go-cdnhealth branch from a650da2 to 04fc4fb Compare June 23, 2018 16:20

rob05c force-pushed the to-go-cdnhealth branch from 04fc4fb to 379389c Compare June 23, 2018 17:19

rob05c mentioned this pull request Jun 23, 2018

Add TO Go deliveryservices/id/health #2458

Merged

rob05c force-pushed the to-go-cdnhealth branch 2 times, most recently from 8b9adb6 to 7ec9108 Compare June 23, 2018 18:50

rob05c mentioned this pull request Jun 24, 2018

Add TO Go deliveryservices/id/capacity #2459

Merged

rob05c force-pushed the to-go-cdnhealth branch from 7ec9108 to 2a04b2f Compare June 25, 2018 17:11

dneuman64 reviewed Jun 25, 2018

View reviewed changes

rob05c force-pushed the to-go-cdnhealth branch from 2a04b2f to 5b96260 Compare July 5, 2018 19:49

rob05c force-pushed the to-go-cdnhealth branch from 5b96260 to 800af8b Compare July 9, 2018 16:49

rob05c force-pushed the to-go-cdnhealth branch from 800af8b to 66da185 Compare July 18, 2018 14:49

mitchell852 added the Traffic Ops related to Traffic Ops label Sep 13, 2019

mitchell852 added this to the Go Rewrite milestone Sep 13, 2019

mitchell852 added tech debt rework due to choosing easy/limited solution and removed new feature A new feature, capability or behavior labels Sep 13, 2019

mitchell852 self-assigned this Sep 13, 2019

rob05c force-pushed the to-go-cdnhealth branch from 3f7eae2 to 9308805 Compare September 16, 2019 14:03

mhoppa reviewed Sep 16, 2019

View reviewed changes

mitchell852 previously approved these changes Sep 18, 2019

View reviewed changes

mitchell852 mentioned this pull request Sep 19, 2019

Fix using http2 between TO and TM when setting proxy #3927

Merged

7 tasks

mitchell852 suggested changes Sep 25, 2019

View reviewed changes

mitchell852 removed their assignment Dec 3, 2019

rob05c force-pushed the to-go-cdnhealth branch from 9308805 to 1b2474e Compare December 11, 2019 16:42

rob05c requested review from mitchell852 and ocket8888 December 11, 2019 17:10

rob05c force-pushed the to-go-cdnhealth branch from 1b2474e to e91e64f Compare December 11, 2019 21:19

Add TO Go cdns/health, cdns/name/health

3e8cdb6

rob05c force-pushed the to-go-cdnhealth branch from e91e64f to 3e8cdb6 Compare December 11, 2019 21:21

mhoppa approved these changes Dec 11, 2019

View reviewed changes

mitchell852 approved these changes Dec 12, 2019

View reviewed changes

mitchell852 merged commit 0b02a3c into apache:master Dec 12, 2019

Uh oh!

Conversation

rob05c commented May 19, 2018 • edited by ocket8888 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asfgit commented May 19, 2018

Uh oh!

asfgit commented May 20, 2018

Uh oh!

asfgit commented May 22, 2018

Uh oh!

asfgit commented Jun 5, 2018

Uh oh!

asfgit commented Jun 23, 2018

Uh oh!

asfgit commented Jun 23, 2018

Uh oh!

asfgit commented Jun 23, 2018

Uh oh!

asfgit commented Jun 25, 2018

Uh oh!

dneuman64 Jun 25, 2018

Choose a reason for hiding this comment

Uh oh!

rob05c Jun 26, 2018

Choose a reason for hiding this comment

Uh oh!

dneuman64 commented Jun 25, 2018

Uh oh!

rob05c commented Jun 26, 2018

Uh oh!

asfgit commented Jul 5, 2018

Uh oh!

asfgit commented Jul 9, 2018

Uh oh!

asfgit commented Jul 19, 2018

Uh oh!

asf-ci commented Sep 16, 2019

Uh oh!

mhoppa left a comment

Choose a reason for hiding this comment

Uh oh!

mhoppa Sep 16, 2019

Choose a reason for hiding this comment

Uh oh!

rob05c Sep 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rob05c Sep 16, 2019

Choose a reason for hiding this comment

Uh oh!

rob05c commented Sep 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mitchell852 commented Sep 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhoppa commented Sep 19, 2019

Uh oh!

mitchell852 left a comment

Choose a reason for hiding this comment

Uh oh!

rob05c commented Sep 27, 2019

Uh oh!

asf-ci commented Dec 11, 2019

Uh oh!

rob05c commented Dec 11, 2019

Uh oh!

asf-ci commented Dec 11, 2019

Uh oh!

asf-ci commented Dec 11, 2019

Uh oh!

mhoppa left a comment

Choose a reason for hiding this comment

Uh oh!

mitchell852 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

rob05c commented May 19, 2018 •

edited by ocket8888

Loading

rob05c Sep 16, 2019 •

edited

Loading

rob05c commented Sep 16, 2019 •

edited

Loading

mitchell852 commented Sep 18, 2019 •

edited

Loading